Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue #47 : Glossary implementation #82

Merged
merged 9 commits into from
Mar 7, 2024
Merged

Issue #47 : Glossary implementation #82

merged 9 commits into from
Mar 7, 2024

Conversation

Ciloe
Copy link
Contributor

@Ciloe Ciloe commented Mar 6, 2024 •
edited
Loading

Add glossary feature :

Issues :

#47

Change log :

  • Add classes related to documentation
  • Change the DeeplRequestHandlerInterface and added to functions :
    • getAcceptHeader required in the entries list in glossary
    • getAuthHeader all glossaries request doesn't work with the auth in body. The header is require
    • I added an abstract to don't change all other Handler classes
  • Add client functions
  • Add models (naming by the documentation sections)
  • Add tests (waiting first review for implementation and naming)

@usox usox requested review from WorksDev and usox March 6, 2024 14:42
@usox
Copy link
Member

usox commented Mar 6, 2024

@WorksDev Would you mind taking a look at it here as well? Thanks.

@usox usox added the enhancement New feature or request label Mar 6, 2024
@usox usox added this to the 3.3 milestone Mar 6, 2024
WorksDev
WorksDev requested changes Mar 6, 2024
View reviewed changes
Copy link
Contributor

@WorksDev WorksDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Ciloe

thank you very much for your work/support - from my point of view there is nothing against it - just a few minor comments.

src/DeeplClient.php Outdated Show resolved Hide resolved
src/Enum/GlossarySubmissionEntryFormatEnum.php Show resolved Hide resolved
src/Handler/DeeplGlossaryCreateRequestHandler.php Show resolved Hide resolved
@Ciloe
Copy link
Contributor Author

Ciloe commented Mar 7, 2024

I fixed the review. If there is no more feedback, I will start dev tests

usox
usox requested changes Mar 7, 2024
View reviewed changes
Copy link
Member

@usox usox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with it so far, the only thing I was a bit confused about was that in the existing requests the auth-key is transferred via body and you have now added the transfer via header - I thought that would already happen that way.

We should do this in principle with V4 and then also carry out the PHP upgrade.

So you have the green light to start testing now.

Thank you

@Ciloe
Copy link
Contributor Author

Ciloe commented Mar 7, 2024

I agree with it so far, the only thing I was a bit confused about was that in the existing requests the auth-key is transferred via body and you have now added the transfer via header - I thought that would already happen that way.

I tested the library on our project. With the auth_key in the body, I faced an error 401. But with the headers it's working....

@usox
Copy link
Member

usox commented Mar 7, 2024

I agree with it so far, the only thing I was a bit confused about was that in the existing requests the auth-key is transferred via body and you have now added the transfer via header - I thought that would already happen that way.

I tested the library on our project. With the auth_key in the body, I faced an error 401. But with the headers it's working....

Yes, and that's how it should be. I'm just wondering why we didn't use the header from the start - probably for some historical reason... 🤷‍♂️

@Ciloe
Copy link
Contributor Author

Ciloe commented Mar 7, 2024

Tests done \o/

@Ciloe Ciloe requested review from usox and WorksDev March 7, 2024 15:49
@Ciloe
Copy link
Contributor Author

Ciloe commented Mar 7, 2024

For a new PR :

  • Passing the auth_key exclusively in headers
  • Add possibility to use json content instead of form x-www-form-urlencoded and let the user chose the right way

@usox usox merged commit a48decc into SC-Networks:master Mar 7, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WorksDev WorksDev WorksDev approved these changes

@usox usox usox approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.
3 participants
@Ciloe @usox @WorksDev